Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Add a better search UI for patients index page #8691 #8834

Open
wants to merge 66 commits into
base: develop
Choose a base branch
from

Conversation

i0am0arunava
Copy link
Contributor

@i0am0arunava i0am0arunava commented Oct 20, 2024

Proposed Changes

@ohcnetwork/care-fe-code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new search component that allows users to search by multiple fields, enhancing search functionality.
    • Added a command interface for improved user interaction and accessibility.
    • Implemented customizable dialog components for better user experience.
    • Enhanced phone number input with improved error handling and visibility options.
    • Added new components for popover and scroll area functionality.
    • Streamlined patient management with a unified search interface.
  • Bug Fixes

    • Resolved issues related to the search functionality and phone number validation.
  • Localization

    • Updated English and Marathi localization files to improve clarity and add new search-related keys.
  • Chores

    • Updated dependencies in package.json to support new features and components.

@i0am0arunava i0am0arunava requested a review from a team as a code owner October 20, 2024 05:39
Copy link

netlify bot commented Oct 20, 2024

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit 84c1155
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/674591224bab88000836f50e
😎 Deploy Preview https://deploy-preview-8834--care-ohc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

src/Components/Form/FormFields/FormField.tsx Outdated Show resolved Hide resolved
src/Components/Form/FormFields/PhoneNumberFormField.tsx Outdated Show resolved Hide resolved
src/Components/Common/SearchByMultipleFields.tsx Outdated Show resolved Hide resolved
Copy link
Member

@bodhish bodhish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should test it out as I am sure we haven't captured all edge cases here

src/Components/Form/FormFields/FormField.tsx Outdated Show resolved Hide resolved
src/Components/Common/SearchByMultipleFields.tsx Outdated Show resolved Hide resolved
inputClassName,
]);

return (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component should have a default state where it says a simple help text. EG
image

@bodhish
Copy link
Member

bodhish commented Oct 21, 2024

@nihal467 do test it

Also @i0am0arunava please add a good title for the PR

@i0am0arunava i0am0arunava changed the title Issues/8691 add better serchui Fixed Add a better search UI for patients index page #8691 Oct 21, 2024
@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Oct 21, 2024
Copy link

👋 Hi, @i0am0arunava,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@vinutv
Copy link
Member

vinutv commented Oct 21, 2024

Screenshot 2024-10-21 at 2 15 39 PM

It's great that shortcut keys are available to access the search type from the dropdown. The dropdown should also be navigable using the up and down arrow keys on the keyboard.

@vinutv
Copy link
Member

vinutv commented Oct 21, 2024

Screenshot 2024-10-21 at 2 19 30 PM

The error message is hidden under the search buttons.

@vinutv
Copy link
Member

vinutv commented Oct 21, 2024

Screenshot 2024-10-21 at 2 25 25 PM

Typed "/" and selected "Phone Number," but the phone number field didn't get focused. It should focus on the phone number input to make it typable when the search type is selected. However, the focus remains on the search button.

@nihal467
Copy link
Member

nihal467 commented Oct 21, 2024

@i0am0arunava can you fix the lint, resolve conflicts, and make the requested changes to get it ready for review

@i0am0arunava
Copy link
Contributor Author

@i0am0arunava can you fix the lint, resolve conflicts, and make the requested changes to get it ready for review

Yes, give me some time,

@i0am0arunava
Copy link
Contributor Author

@i0am0arunava can you fix the lint, resolve conflicts, and make the requested changes to get it ready for review

done

@i0am0arunava
Copy link
Contributor Author

@rithviknishad,@bodhish ,@nihal467, I have made changes and raised pr please review .

@bodhish
Copy link
Member

bodhish commented Oct 22, 2024

I don't see any of the changes Vinu requested implemented!

@bodhish
Copy link
Member

bodhish commented Oct 22, 2024

Also did you break the mobile input its showing IN instead of the country logo 🤔

Copy link
Contributor Author

@i0am0arunava i0am0arunava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implemented debouncing, I found implementing import { debounce } from "lodash-es"; to be very complex and it wasn't working properly. After that, I switched to using import { useDebounce } from "use-debounce";, which worked properly.

@@ -317,16 +292,10 @@ export const PatientManager = () => {
return cleanedData;
};

const [debouncedParams] = useDebounce(params, 1000); // Debounce time in milliseconds
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found implementing import { debounce } from "lodash-es"; to be very complex and it wasn't working properly. After that, I switched to using import { useDebounce } from "use-debounce";, which worked properly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you verified it to be working as needed, it is fine.

@github-actions github-actions bot removed the merge conflict pull requests with merge conflict label Nov 20, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (4)
src/hooks/useDebounce.ts (2)

3-10: Update JSDoc to match TypeScript types

The JSDoc parameter types don't match the TypeScript type annotations. The value parameter is typed as string in the implementation but documented as any.

 /**
  * Custom hook to debounce a value.
  *
- * @param {any} value - The value to debounce.
+ * @param {string} value - The value to debounce.
  * @param {number} delay - The debounce delay in milliseconds.
- * @returns {any} - The debounced value.
+ * @returns {string} - The debounced value.
  */

1-50: Consider performance implications and provide usage guidance

As this hook is used for search functionality, consider:

  1. Document recommended delay values (e.g., 300ms for search, 500ms for other use cases)
  2. Add usage examples in comments
  3. Consider memoizing the returned value for performance optimization when used with complex components

Add usage documentation:

/**
 * @example
 * // Basic usage with search input
 * const SearchComponent = () => {
 *   const [searchTerm, setSearchTerm] = useState('');
 *   const debouncedSearch = useDebounce(searchTerm, 300);
 *
 *   useEffect(() => {
 *     // Perform search with debouncedSearch
 *   }, [debouncedSearch]);
 *
 *   return <input onChange={(e) => setSearchTerm(e.target.value)} />;
 * };
 */
src/components/Patient/ManagePatients.tsx (2)

752-785: Enhance type safety and internationalization of search options.

Consider the following improvements:

  1. Define a proper type for search options instead of inline type assertions
  2. Use translation function for labels and placeholders
  3. Consider using a more flexible phone number validation approach

Apply this diff to improve the implementation:

+type SearchOptionType = 'text' | 'phone';
+
+interface SearchOption {
+  key: string;
+  label: string;
+  type: SearchOptionType;
+  placeholder: string;
+  value: string;
+  shortcutKey: string;
+}
+
 const searchOptions: SearchOption[] = [
   {
     key: "phone_number",
-    label: "Phone Number",
+    label: t("phone_number"),
     type: "phone",
-    placeholder: "Search_by_phone_number",
+    placeholder: t("search_by_phone_number"),
     value: qParams.phone_number || "",
     shortcutKey: "p",
   },
   // ... similar changes for other options
 ];

998-998: Consider using design system tokens for spacing.

Instead of hardcoding padding values, consider using your design system's spacing tokens for better maintainability and consistency.

-className="pb-12"
+className="pb-spacing-12"  // Assuming your design system uses spacing tokens
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between acc6362 and fa58519.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • src/components/Patient/ManagePatients.tsx (7 hunks)
  • src/hooks/useDebounce.ts (1 hunks)
🔇 Additional comments (1)
src/hooks/useDebounce.ts (1)

50-50: LGTM!

The export statement follows the correct convention.

Comment on lines 11 to 26
function compareObjects(obj1: string, obj2: string) {
obj1 = JSON.parse(obj1);
obj2 = JSON.parse(obj2);
const entries1 = Object.entries(obj1).map(([key, value]) => [
key,
String(value),
]);
const entries2 = Object.entries(obj2).map(([key, value]) => [
key,
String(value),
]);
entries1.sort(([key1], [key2]) => key1.localeCompare(key2));
entries2.sort(([key1], [key2]) => key1.localeCompare(key2));

return JSON.stringify(entries1) === JSON.stringify(entries2);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling and improve type safety

The compareObjects function has several potential improvements:

  1. Missing error handling for JSON.parse
  2. Function should be moved outside the hook to prevent recreation
  3. Type safety could be improved
-function compareObjects(obj1: string, obj2: string) {
+function compareObjects(obj1: string, obj2: string): boolean {
+  try {
     obj1 = JSON.parse(obj1);
     obj2 = JSON.parse(obj2);
+  } catch (error) {
+    console.error('Invalid JSON format:', error);
+    return false;
+  }

   const entries1 = Object.entries(obj1).map(([key, value]) => [
     key,
     String(value),
   ]);
   const entries2 = Object.entries(obj2).map(([key, value]) => [
     key,
     String(value),
   ]);
   entries1.sort(([key1], [key2]) => key1.localeCompare(key2));
   entries2.sort(([key1], [key2]) => key1.localeCompare(key2));

   return JSON.stringify(entries1) === JSON.stringify(entries2);
 }

Also, move this function outside the hook to prevent unnecessary recreation on each render:

const compareObjects = (obj1: string, obj2: string): boolean => {
  // ... implementation
};

const useDebounce = (value: string, delay: number) => {
  // ... hook implementation
};

Comment on lines 28 to 45
const [debouncedValue, setDebouncedValue] = useState(value);
const previousValueRef = useRef(value);
console.log(value);
useEffect(() => {
if (compareObjects(previousValueRef.current, value)) {
return;
}

const handler = setTimeout(() => {
setDebouncedValue(value);
previousValueRef.current = value;
}, delay);

// Cleanup the timeout if the value changes or the component unmounts
return () => {
clearTimeout(handler);
};
}, [value, delay]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove debug console.log and validate dependencies

The hook implementation has a few issues to address:

  1. Remove the debug console.log statement
  2. Consider validating the delay parameter
 const [debouncedValue, setDebouncedValue] = useState(value);
 const previousValueRef = useRef(value);
-console.log(value);
+
+if (delay < 0) {
+  console.warn('Debounce delay should not be negative');
+}

 useEffect(() => {

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor Author

@i0am0arunava i0am0arunava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay now ?

* @returns {any} - The debounced value.
*/
const useDebounce = (value: string, delay: number) => {
function compareObjects(obj1: string, obj2: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I observed that two parameters coming in differently but looking the same were being treated as different values. This was causing unnecessary API calls. Upon examining the previous parameter value and the current incoming parameter value, I found they were different. This issue was occurring not only when typing inside the phone number field but also when changing the option, causing unnecessary API calls.

{page: 1, limit: 12, name: undefined, patient_no: undefined, is_active: 'True', …}
{page: '1', limit: 12, name: undefined, patient_no: undefined, is_active: 'True', …}

If you observe the page key value, the previous one is a number, and the current one is a string. This was the problem. I solved it by comparing two objects while ignoring their value types, which now prevents unnecessary API calls

@nihal467 nihal467 removed question Further information is requested needs-triage labels Nov 25, 2024
@nihal467
Copy link
Member

@shivankacker can you review this PR

@i0am0arunava
Copy link
Contributor Author

@shivankacker can you review this PR

@Nihal

@Shivank Kacker
, I want to inform that some changes I have not done yet in my pr, I want some clarification,I will discuss about that in today's meeting

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
src/components/Common/SearchByMultipleFields.tsx (1)

48-48: Enhance type safety for clearSearch prop

The clearSearch prop type could be more specific and type-safe.

-clearSearch?: { value: boolean; params?: string[] };
+interface ClearSearchConfig {
+  value: boolean;
+  params: string[];
+}
+clearSearch?: ClearSearchConfig;
src/components/Patient/ManagePatients.tsx (1)

757-790: Add type safety to searchOptions array.

Consider defining an interface for the search options and using a const assertion for better type safety:

+interface SearchOption {
+  key: 'phone_number' | 'name' | 'patient_no' | 'emergency_phone_number';
+  label: string;
+  type: 'phone' | 'text';
+  placeholder: string;
+  value: string;
+  shortcutKey: string;
+}

-const searchOptions = [
+const searchOptions: SearchOption[] = [
   {
     key: "phone_number",
     // ... rest of the options
   }
 ] as const;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between fa58519 and 9102b20.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • package.json (2 hunks)
  • public/locale/en.json (4 hunks)
  • src/components/Common/SearchByMultipleFields.tsx (1 hunks)
  • src/components/Patient/ManagePatients.tsx (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json
🔇 Additional comments (9)
src/components/Common/SearchByMultipleFields.tsx (4)

51-54: Replace custom EventType with built-in React types

As suggested in previous reviews, replace the custom EventType with React's built-in types for better type safety.

-type EventType = {
-  value: string;
-  target?: { value: string };
-};
+import { FieldChangeEvent } from "@/components/Form/FormFields/Utils";

Usage example:

// For regular inputs
onChange: (e: React.ChangeEvent<HTMLInputElement>) => handleSearchChange(e.target.value)
// For phone input
onChange: (e: FieldChangeEvent<string>) => handleSearchChange(e.value)

38-38: Improve component prop type in SearchOption interface

The component prop should specify proper props interface instead of HTMLDivElement.

-component?: React.ComponentType<HTMLDivElement>;
+component?: React.ComponentType<{ className?: string }>;

239-243: Improve error message handling

The error message assumes phone number validation, but other input types might have different error messages.

 {error && (
   <div className="error-text px-2 mb-1 text-xs font-medium tracking-wide text-danger-500 transition-opacity duration-300">
-    {t("invalid_phone_number")}
+    {typeof error === 'string' ? t(error) : t(`invalid_${selectedOption.type}_input`)}
   </div>
 )}

226-226: Use option.label instead of option.key for display text

Using the key for display text may not provide the correct translation.

-<span className="flex-1">{t(option.key)}</span>
+<span className="flex-1">{t(option.label)}</span>

-{t(option.key)}
+{t(option.label)}

Also applies to: 258-258

src/components/Patient/ManagePatients.tsx (2)

190-195: LGTM: Debouncing implementation looks good!

The implementation of debounced search parameters with a 1-second delay is a good approach to prevent excessive API calls during rapid user input.

Also applies to: 301-301


Line range hint 1003-1007: LGTM: CountBlock height adjustment.

The CountBlock component's height has been adjusted with pb-12 class to match the search component's height, which aligns with the PR objectives.

public/locale/en.json (3)

630-630: LGTM! Consistent terminology for phone numbers.

The change from "emergency_contact_number" to "emergency_phone_number" maintains consistency with other phone number related keys in the file.


995-995: LGTM! More descriptive label.

Changed "OP/IP No" to "OP/IP Number" for better clarity and consistency with other number-related labels.


1144-1147: LGTM! New search labels support multi-criteria search.

The added search labels align with the PR objective to implement a smart search component with multiple search criteria:

  • Emergency Contact Phone Number
  • Patient Name
  • Phone Number
  • UHID

src/components/Common/SearchByMultipleFields.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
src/components/Patient/ManagePatients.tsx (2)

757-790: Enhance type safety and internationalization for search options.

Consider these improvements:

  1. Define a type for the search options to ensure type safety
  2. Use translation keys for labels to support internationalization
+type SearchOptionType = 'text' | 'phone';
+type SearchOption = {
+  key: string;
+  label: string;
+  type: SearchOptionType;
+  placeholder: string;
+  value: string;
+  shortcutKey: string;
+};

 const searchOptions = [
   {
     key: "phone_number",
-    label: "Phone Number",
+    label: t("phone_number"),
     type: "phone" as const,
     placeholder: "Search_by_phone_number",
     value: qParams.phone_number || "",
     shortcutKey: "p",
   },
   // ... rest of the options with similar changes
 ] as const;

190-195: Consider using useDebounce hook for better reusability.

The current debouncing implementation could be improved using a custom hook or the useDebounce hook from a library like use-debounce.

-const [debouncedSearchParams, setDebouncedSearchParams] = useState(params);
-
-useEffect(() => {
-  const timeout = setTimeout(() => setDebouncedSearchParams(params), 1000);
-  return () => clearTimeout(timeout);
-}, [params]);
+import { useDebounce } from 'use-debounce';
+
+const [debouncedSearchParams] = useDebounce(params, 1000);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9102b20 and a9746cc.

📒 Files selected for processing (3)
  • public/locale/en.json (3 hunks)
  • src/components/Common/Page.tsx (2 hunks)
  • src/components/Patient/ManagePatients.tsx (7 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/components/Common/Page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • public/locale/en.json
🔇 Additional comments (2)
src/components/Patient/ManagePatients.tsx (2)

792-814: ⚠️ Potential issue

Critical improvements needed in search handler implementation.

The current implementation has several issues:

  1. Phone number validation is too rigid (requires exactly 13 characters)
  2. Complex nested ternary operations reduce readability
  3. Missing type safety for the key parameter

Apply this refactor to address the issues:

+type SearchKey = 'phone_number' | 'name' | 'patient_no' | 'emergency_phone_number';
+
+const isValidPhoneNumber = (value: string) => {
+  const phoneRegex = /^\+?[\d\s-]{10,15}$/;
+  return value === '' || phoneRegex.test(value);
+};
+
 const handleSearch = useCallback(
-  (key: string, value: string) => {
+  (key: SearchKey, value: string) => {
     const updatedQuery = {
-      phone_number:
-        key === "phone_number"
-          ? value.length >= 13 || value === ""
-            ? value
-            : undefined
-          : undefined,
+      phone_number: key === "phone_number" && isValidPhoneNumber(value) ? value : undefined,
       name: key === "name" ? value : undefined,
       patient_no: key === "patient_no" ? value : undefined,
-      emergency_phone_number:
-        key === "emergency_phone_number"
-          ? value.length >= 13 || value === ""
-            ? value
-            : undefined
-          : undefined,
+      emergency_phone_number: key === "emergency_phone_number" && isValidPhoneNumber(value) ? value : undefined,
     };

     updateQuery(updatedQuery);
   },
   [updateQuery],
 );

Likely invalid or redundant comment.


1014-1018: Verify SearchByMultipleFields component interface.

The clearSearch prop is being passed but might not be defined in the component's interface based on previous review comments.

✅ Verification successful

The clearSearch prop is correctly defined in the component's interface

The SearchByMultipleFields component's interface properly defines the clearSearch prop as an optional prop with the following type:

clearSearch?: { value: boolean; params?: string[] }

The implementation also correctly handles this prop in the component's logic through a useEffect hook that clears the search input when specified conditions are met.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify SearchByMultipleFields component implementation
# Test: Check if the component exists and has the expected props interface

# Look for the component definition
ast-grep --pattern 'interface $SearchProps {
  $$$
  options: $_;
  onSearch: $_;
  clearSearch?: $_;
  $$$
}'

Length of output: 112


Script:

#!/bin/bash
# Let's try a broader search for the SearchByMultipleFields component and its interface
# First, find the file containing the component
fd "SearchByMultipleFields" --type f

# Then let's look for any interface or type definition related to this component
rg -A 5 "interface.*Props.*{|type.*Props.*=" --type ts --type tsx

Length of output: 289


Script:

#!/bin/bash
# Now that we found the component file, let's examine its contents
cat src/components/Common/SearchByMultipleFields.tsx

# Also search for interface definitions with fixed regex
rg "interface.*Props" --type ts --type tsx

Length of output: 7886

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
src/components/Common/SearchByMultipleFields.tsx (2)

73-73: Improve error state type definition

The error state type is ambiguous with string | undefined | boolean. Consider using a more specific type:

-const [error, setError] = useState<string | undefined | boolean>();
+type ValidationError = { message: string; field: string } | null;
+const [error, setError] = useState<ValidationError>(null);

199-205: Enhance accessibility attributes

Add more descriptive ARIA attributes:

 <div
   role="searchbox"
   aria-expanded={open}
   aria-controls="search-options"
   aria-haspopup="listbox"
+  aria-label={t('search_by_multiple_fields')}
+  aria-describedby={error ? 'search-error' : undefined}
   className="flex items-center rounded-t-lg"
 >
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 241a1f1 and 84c1155.

📒 Files selected for processing (1)
  • src/components/Common/SearchByMultipleFields.tsx (1 hunks)
🔇 Additional comments (3)
src/components/Common/SearchByMultipleFields.tsx (3)

38-38: 🛠️ Refactor suggestion

Replace HTMLDivElement with proper component props type

The component prop type should be more specific:

-component?: React.ComponentType<HTMLDivElement>;
+component?: React.ComponentType<{ className?: string }>;

Likely invalid or redundant comment.


51-54: 🛠️ Refactor suggestion

Replace custom EventType with React's built-in types

Use React's built-in event types for better type safety:

-type EventType = {
-  value: string;
-  target?: { value: string };
-};
+import { FieldChangeEvent } from "@/components/Form/FormFields/Utils";

// Usage:
// For regular inputs: React.ChangeEvent<HTMLInputElement>
// For phone input: FieldChangeEvent<string>

Likely invalid or redundant comment.


244-248: 🛠️ Refactor suggestion

Use translation key for error message

The error message should use the error state's message:

 {error && (
   <div className="error-text px-2 mb-1 text-xs font-medium tracking-wide text-danger-500 transition-opacity duration-300">
-    {t("invalid_phone_number")}
+    {t(error.message)}
   </div>
 )}

Likely invalid or redundant comment.

Comment on lines +168 to +180
switch (selectedOption.type) {
case "phone":
return (
<PhoneNumberFormField
name={selectedOption.key}
placeholder={t(selectedOption.placeholder)}
types={["mobile", "landline"]}
{...commonProps}
errorClassName="hidden"
hideHelp={true}
onError={(error: FieldError) => setError(error)}
/>
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling for different input types

The error handling should be generic for all input types:

 case "phone":
   return (
     <PhoneNumberFormField
       name={selectedOption.key}
       placeholder={t(selectedOption.placeholder)}
       types={["mobile", "landline"]}
       {...commonProps}
       errorClassName="hidden"
       hideHelp={true}
-      onError={(error: FieldError) => setError(error)}
+      onError={(error: FieldError) => 
+        setError(error ? { message: `invalid_${selectedOption.type}_input`, field: selectedOption.key } : null)
+      }
     />
   );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
switch (selectedOption.type) {
case "phone":
return (
<PhoneNumberFormField
name={selectedOption.key}
placeholder={t(selectedOption.placeholder)}
types={["mobile", "landline"]}
{...commonProps}
errorClassName="hidden"
hideHelp={true}
onError={(error: FieldError) => setError(error)}
/>
);
switch (selectedOption.type) {
case "phone":
return (
<PhoneNumberFormField
name={selectedOption.key}
placeholder={t(selectedOption.placeholder)}
types={["mobile", "landline"]}
{...commonProps}
errorClassName="hidden"
hideHelp={true}
onError={(error: FieldError) =>
setError(error ? { message: `invalid_${selectedOption.type}_input`, field: selectedOption.key } : null)
}
/>
);

Comment on lines +98 to +140
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
if (
e.key === "/" &&
!(document.activeElement instanceof HTMLInputElement)
) {
e.preventDefault();
setOpen(true);
}
if (open) {
if (e.key === "ArrowDown") {
setFocusedIndex((prevIndex) =>
prevIndex === options.length - 1 ? 0 : prevIndex + 1,
);
} else if (e.key === "ArrowUp") {
setFocusedIndex((prevIndex) =>
prevIndex === 0 ? options.length - 1 : prevIndex - 1,
);
} else if (e.key === "Enter") {
handleOptionChange(options[focusedIndex]);
}

if (e.key === "Escape") {
inputRef.current?.focus();
setOpen(false);
}

options.forEach((option) => {
if (
e.key.toLocaleLowerCase() ===
option.shortcutKey.toLocaleLowerCase() &&
open
) {
e.preventDefault();
handleOptionChange(option);
}
});
}
};

document.addEventListener("keydown", handleKeyDown);
return () => document.removeEventListener("keydown", handleKeyDown);
}, [focusedIndex, open, handleOptionChange, options]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize keyboard event handler implementation

Move the handleKeyDown function outside useEffect and memoize it with useCallback:

+const handleKeyDown = useCallback((e: KeyboardEvent) => {
-const handleKeyDown = (e: KeyboardEvent) => {
   if (e.key === "/" && !(document.activeElement instanceof HTMLInputElement)) {
     e.preventDefault();
     setOpen(true);
   }
   // ... rest of the handler
-};
+}, [focusedIndex, open, handleOptionChange, options]);

 useEffect(() => {
   document.addEventListener("keydown", handleKeyDown);
   return () => document.removeEventListener("keydown", handleKeyDown);
-}, [focusedIndex, open, handleOptionChange, options]);
+}, [handleKeyDown]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
if (
e.key === "/" &&
!(document.activeElement instanceof HTMLInputElement)
) {
e.preventDefault();
setOpen(true);
}
if (open) {
if (e.key === "ArrowDown") {
setFocusedIndex((prevIndex) =>
prevIndex === options.length - 1 ? 0 : prevIndex + 1,
);
} else if (e.key === "ArrowUp") {
setFocusedIndex((prevIndex) =>
prevIndex === 0 ? options.length - 1 : prevIndex - 1,
);
} else if (e.key === "Enter") {
handleOptionChange(options[focusedIndex]);
}
if (e.key === "Escape") {
inputRef.current?.focus();
setOpen(false);
}
options.forEach((option) => {
if (
e.key.toLocaleLowerCase() ===
option.shortcutKey.toLocaleLowerCase() &&
open
) {
e.preventDefault();
handleOptionChange(option);
}
});
}
};
document.addEventListener("keydown", handleKeyDown);
return () => document.removeEventListener("keydown", handleKeyDown);
}, [focusedIndex, open, handleOptionChange, options]);
const handleKeyDown = useCallback((e: KeyboardEvent) => {
if (
e.key === "/" &&
!(document.activeElement instanceof HTMLInputElement)
) {
e.preventDefault();
setOpen(true);
}
if (open) {
if (e.key === "ArrowDown") {
setFocusedIndex((prevIndex) =>
prevIndex === options.length - 1 ? 0 : prevIndex + 1,
);
} else if (e.key === "ArrowUp") {
setFocusedIndex((prevIndex) =>
prevIndex === 0 ? options.length - 1 : prevIndex - 1,
);
} else if (e.key === "Enter") {
handleOptionChange(options[focusedIndex]);
}
if (e.key === "Escape") {
inputRef.current?.focus();
setOpen(false);
}
options.forEach((option) => {
if (
e.key.toLocaleLowerCase() ===
option.shortcutKey.toLocaleLowerCase() &&
open
) {
e.preventDefault();
handleOptionChange(option);
}
});
}
}, [focusedIndex, open, handleOptionChange, options]);
useEffect(() => {
document.addEventListener("keydown", handleKeyDown);
return () => document.removeEventListener("keydown", handleKeyDown);
}, [handleKeyDown]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review required
Development

Successfully merging this pull request may close these issues.

Add a better search UI for patients index page
8 participants